Introduce package manager command classes - #1621
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (12)
src/managers/conda/condaPackageManager.ts:137
- CondaListCommand.execute() requires an environmentPath to build
conda list -p <env> --json, but getPackages() calls it with no args, which always returns an empty list (see CondaListCommand.execute() guard). This will break package listing/caching for conda environments.
src/managers/conda/commands/install.ts:26 - This command builds
conda install ...without targeting the selected environment (no--prefix <envPath>/-p <envPath>). As a result, installs will apply to whatever conda considers the current/default environment (often base) rather than the provided PythonEnvironment.
protected buildCommand(executeArgs: InstallExecuteArgs): string[] {
let args = ['install', '-y', '-c', 'conda-forge'];
if (executeArgs.upgrade) {
args.push('--upgrade');
}
src/managers/conda/commands/uninstall.ts:21
- This command builds
conda remove -y ...without specifying the target environment (missing--prefix <envPath>/-p <envPath>). This can uninstall packages from the wrong environment (typically base).
protected buildCommand(executeArgs: UninstallExecuteArgs): string[] {
return ['remove', '-y', ...executeArgs.packages.map((pkg) => pkg.packageName)];
}
src/managers/base/commands/packageManagerCommand.ts:60
- executeWithProgress() shows notification progress but does not set
cancellable: trueand does not propagate the progress cancellation token into command execution. This is a regression from prior patterns that used cancellable progress and a token, and means users can't cancel long-running package operations from the notification UI.
public executeWithProgress<T = unknown, A extends BaseExecuteArgs = BaseExecuteArgs>(
executeArgs?: A,
title?: string,
): Promise<T> {
if (!executeArgs?.showProgress) {
return this.execute(executeArgs) as Promise<T>;
}
return Promise.resolve(
window.withProgress(
{
location: ProgressLocation.Notification,
title: title ?? l10n.t('Running package manager command'),
},
() => this.execute(executeArgs) as Promise<T>,
),
);
src/managers/base/commands/packageManagerCommand.ts:39
- The command-specific configuration section is loaded into
this.config, but it is never applied to behavior. In particular,timeoutstays hard-coded to 300000 even though the PR introducesexecutionTimeoutsettings in package.json for these command types. This makes the new settings ineffective.
constructor(options: CommandConstructorOptions) {
this.pythonExecutable = options.pythonExecutable;
this.log = options.log;
const configSection = options.configSection ?? (this.constructor as typeof PackageManagerCommand).configSection;
this.config = configSection ? getConfiguration(`python-envs.packageManager.${configSection}`) : undefined;
}
src/managers/poetry/commands/show.ts:4
- poetry command classes import
runPoetryfrompoetryPackageManager, butpoetryPackageManageralso imports these command classes (via ./commands/index). This creates a circular dependency that can leaverunPoetryundefined/partially initialized at runtime.
src/managers/poetry/commands/add.ts:3 - This file imports
runPoetryfrompoetryPackageManagerwhilepoetryPackageManagerimports this command via ./commands/index, creating a circular dependency that can result in undefined imports at runtime.
src/managers/builtin/commands/list.ts:29 - PipListCommand JSON.parse() is unguarded. If pip writes non-JSON to stdout (warnings, empty output, etc.), this throws and bubbles out of getPackages(), breaking package listing. The previous code paths generally treated parse failures as an empty list rather than throwing.
const parser = (output: string): void => {
const json = JSON.parse(output);
if (!Array.isArray(json)) {
throw new Error('Invalid output from pip list command');
}
src/managers/builtin/commands/listDirectNames.ts:31
- PipListDirectNamesCommand JSON.parse() is unguarded. If stdout isn't valid JSON (warnings/noise), this throws and breaks callers that rely on direct-name discovery; it's safer to treat parse failures as "no results".
const parser = (output: string): void => {
const packages = JSON.parse(output);
if (!Array.isArray(packages)) {
throw new Error('Invalid output from pip list command');
}
directNames = packages.filter(({ name }) => name).map(({ name }) => name);
};
src/managers/builtin/commands/availableVersions.ts:35
- The refactor moved available-version parsing into PipAvailableVersionsCommand/UvAvailableVersionsCommand, but the prior unit tests (pipVersions.unit.test.ts) were removed and there do not appear to be replacement tests covering this parsing/filtering behavior (JSON extraction, prerelease filtering, invalid output handling). This is a regression risk for version selection UX.
const parser = (output: string): void => {
const match = output.match(/{[\s\S]*}/);
if (!match) {
availableVersions = [];
return;
}
SETTINGS_ARCHITECTURE.md:25
- This architecture doc references
src/managers/builtin/commands/commandSettings.tsand a constructor optioncancellationToken, but the implemented base class issrc/managers/base/commands/packageManagerCommand.tsand cancellation is passed via execute args (BaseExecuteArgs.cancellationToken). As written, the doc is out of sync with the code in this PR.
### 1. Base Class
**File**: `src/managers/builtin/commands/commandSettings.ts`
```typescript
interface CommandConstructorOptions {
pythonExecutable: string;
log?: LogOutputChannel;
cancellationToken?: CancellationToken;
}
src/api.ts:1440
- This InstallCommand documentation says implementers should return an exit code, but the actual base type exported from
./managers/base/commands/installdefinesexecute(...): Promise<void>. This mismatch can confuse extension authors consuming the public API.
* Abstract base class for install command implementations.
*
* Implement this interface to support package installation in your package manager.
* The command handles executing the install operation and returning the exit code.
*
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 41 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (8)
src/managers/conda/condaPackageManager.ts:136
- CondaListCommand requires an environment path to build
conda list -p <env> --json. Here it's executed with no args, soCondaListCommand.execute()returns[](see commands/list.ts:34-36), meaning package listing will always be empty and getPackages() will cache an empty list.
src/managers/conda/commands/install.ts:24 - Conda installs need to target the specific environment (previously done via
--prefix <environmentPath>). This command currently runsconda install ...with no--prefix, so it will modify whatever conda considers the “current” environment (often base), not the PythonEnvironment being managed.
protected buildCommand(executeArgs: InstallExecuteArgs): string[] {
let args = ['install', '-y', '-c', 'conda-forge'];
if (executeArgs.upgrade) {
args.push('--upgrade');
src/managers/conda/commands/uninstall.ts:23
- Like installs, conda removals need to target the environment being managed. This command omits
--prefix <environmentPath>, so it may uninstall from the wrong conda environment (often base) when run outside an activated shell.
protected buildCommand(executeArgs: UninstallExecuteArgs): string[] {
return ['remove', '-y', ...executeArgs.packages.map((pkg) => pkg.packageName)];
}
async execute(executeArgs: UninstallExecuteArgs): Promise<void> {
src/managers/conda/condaPackageManager.ts:175
- PackageManager.getPackageAvailableVersions() is documented to return versions newest-first (src/api.ts:734-740). This implementation sorts ascending (oldest-first) and also doesn’t filter out
nullresults frompep440.explain(), which can lead to runtime errors and type unsafety.
src/managers/builtin/pipPackageManager.ts:226 - PackageManager.getPackageAvailableVersions() is documented to return versions newest-first (src/api.ts:734-740). This implementation returns the raw order from pip/uv output (unsorted) and filters out
undefinedeven thoughpep440.explain()returnsnullon parse failure, which can leak nulls into the returned array.
const versionStrings = await availableVersionsCmd.execute({
packageName,
pythonVersion: environment.version,
});
return versionStrings.map((v) => parse(v)).filter((parsed) => parsed !== undefined) as Pep440Version[];
src/managers/base/commands/packageManagerCommand.ts:39
- PackageManagerCommand loads a command-specific configuration section, and package.json introduces
executionTimeoutsettings for each command type, but the value is never applied. As a result, user-configured timeouts have no effect and all commands always use the hardcoded 300000ms timeout.
this.pythonExecutable = options.pythonExecutable;
this.log = options.log;
const configSection = options.configSection ?? (this.constructor as typeof PackageManagerCommand).configSection;
this.config = configSection ? getConfiguration(`python-envs.packageManager.${configSection}`) : undefined;
}
src/managers/base/commands/packageManagerCommand.ts:60
- executeWithProgress() shows a progress notification but does not make it cancellable and does not propagate the progress token into
executeArgs.cancellationToken. This regresses cancellation behavior from the previouswithProgress(..., (_p, token) => ...)call sites and prevents users from cancelling long-running package operations.
window.withProgress(
{
location: ProgressLocation.Notification,
title: title ?? l10n.t('Running package manager command'),
},
() => this.execute(executeArgs) as Promise<T>,
),
);
src/managers/builtin/commands/availableVersions.ts:42
- The pip/uv available-versions JSON parsing and prerelease filtering logic is new here and the previous unit coverage for pip index versions parsing (src/test/managers/builtin/pipVersions.unit.test.ts) has been removed. There should be replacement unit tests covering: valid JSON extraction, missing/invalid JSON returning [], and includePrerelease filtering behavior.
const parser = (output: string): void => {
const match = output.match(/{[\s\S]*}/);
if (!match) {
availableVersions = [];
return;
}
try {
const parsed = JSON.parse(match[0]) as { versions?: string[] };
let versions = Array.isArray(parsed.versions) ? parsed.versions.filter((v) => !!v.trim()) : [];
if (!executeArgs.includePrerelease) {
versions = versions.filter((version) => !/[ab]|rc|dev/i.test(version));
}
availableVersions = versions;
186524a to
14afd71
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/managers/conda/condaPackageManager.ts:183
PackageManager.getPackageAvailableVersions()is documented to return versions “newest first” (src/api.ts:741-742), but this sorts ascending (compare(a, b)). This reverses the expected order and regresses consumers like the version QuickPick.
return versionStrings
.map((v) => parse(v))
.filter((parsed): parsed is Pep440Version => parsed !== null)
.sort((a, b) => compare(a.public, b.public));
695d18b to
9b2683e
Compare
eleanorjboyd
left a comment
There was a problem hiding this comment.
very cool! Great idea getting some comphrehensive structure around the packaging commands! For the real PR would suggest splitting this up- maybe the base and then each package manager on top of that or smth since above 1k is a lot of lines for a PR. Thanks!!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
src/managers/conda/condaPackageManager.ts:193
PackageManager.getPackageAvailableVersions()is documented to return versions "newest first" (src/api.ts:741-743), but this sorts ascending (compare(a, b)), producing oldest-first ordering.
const versionStrings = await availableVersionsCmd.execute({ packageName, pythonVersion: '' });
return versionStrings
.map((v) => parse(v))
.filter((parsed): parsed is Pep440Version => parsed !== null)
.sort((a, b) => compare(a.public, b.public));
src/managers/builtin/commands/factory.ts:13
shouldUseUv()expects an environment path (envPath) to detect uv-managed environments viagetUvEnvironments(), but the factory passesoptions.pythonExecutable(a Python executable path). This breaks the uv-environment detection logic and can cause uv envs to be treated as non-uv envs whenpython-envs.alwaysUseUvis false.
export async function createPipOrUvCommand<T, P extends T, U extends T>(
options: CommandConstructorOptions,
PipCommand: CommandConstructor<P>,
UvCommand: CommandConstructor<U>,
): Promise<T> {
return (await shouldUseUv(options.log, options.pythonExecutable))
? new UvCommand(options)
: new PipCommand(options);
src/managers/builtin/commands/listDirectNames.ts:58
UvListDirectNamesCommanddoes not pass--python <path>when runninguv pip tree, so it may query the wrong interpreter/environment (whatever uv chooses by default) instead of the target environment represented bythis.pythonExecutable. Other uv commands in this refactor consistently include--python this.pythonExecutable.
protected buildCommand(): string[] {
return ['pip', 'tree', '--depth', '1'];
}
async execute(executeArgs?: BaseExecuteArgs): Promise<Set<string>> {
const output = await runUV(
this.buildCommand(),
undefined,
this.log,
executeArgs?.cancellationToken,
this.timeout,
);
src/managers/builtin/pipPackageManager.ts:132
PipPackageManager.manage()swallowsCancellationErrorby returning successfully. Other package managers in this refactor (e.g. conda/poetry) rethrow cancellation so callers/withProgress can treat the operation as cancelled consistently.
} catch (e) {
if (e instanceof CancellationError) {
// Cancellation is a normal control-flow exit; do not surface an error.
return;
}
src/managers/builtin/pipPackageManager.ts:257
- Docstring says uv direct names uses
uv pip list --not-required, but the implementation usesuv pip tree(seeUvListDirectNamesCommandin src/managers/builtin/commands/listDirectNames.ts). This makes the documentation misleading for future maintenance.
/**
* Returns direct (non-transitive) package names using `pip list --not-required` or `uv pip list --not-required`.
*
src/managers/builtin/commands/list.ts:35
- The JSON parsing/validation logic for pip/uv list output moved into
PipListCommand/UvListCommand, but the previous unit tests covering this behavior were removed (pipListUtils.unit.test.ts). Add equivalent unit tests for these command classes (valid JSON, invalid JSON, non-array JSON, filtering missing fields) so regressions in parsing don’t go unnoticed.
async execute(executeArgs?: BaseExecuteArgs): Promise<PackageInfo[]> {
const output = await runPython(
this.pythonExecutable,
this.buildCommand(),
undefined,
this.log,
executeArgs?.cancellationToken,
this.timeout,
);
let json: unknown;
try {
json = JSON.parse(output);
} catch (e) {
this.log?.error(`Failed to parse pip list output: ${e}`);
return [];
}
if (!Array.isArray(json)) {
this.log?.error('Invalid output from pip list command');
return [];
}
492843b to
bd9c3b0
Compare
bd9c3b0 to
b620149
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (12)
src/managers/poetry/commands/runPoetry.ts:45
- If the process emits an 'error' as a result of cancellation/kill, this handler will log it as an execution error and reject with the raw error rather than a CancellationError. Check cancellationRequested first to preserve cancellation semantics (matching runPython/runUV).
proc.on('error', (error) => {
log?.error(`Error executing poetry command: ${error}`);
reject(error);
});
src/managers/builtin/commands/availableVersions.ts:87
- UvAvailableVersionsCommand also needs to enforce newest-first ordering to match the API contract and existing pip implementation. After filtering prereleases, parse and sort using pep440 rcompare before returning.
const parsed = JSON.parse(match[0]) as { versions?: string[] };
return this.filterVersions(
Array.isArray(parsed.versions) ? parsed.versions : [],
executeArgs.includePrerelease,
);
src/managers/poetry/commands/runPoetry.ts:23
- Cancellation handling calls proc.kill() without guarding against kill() throwing (e.g., already-exited process) and doesn’t track cancellation state. This can surface as an unhandled exception during cancellation and make failures look like non-cancellation errors. Align with the runPython/runUV pattern by tracking cancellationRequested and wrapping kill() in try/catch.
This issue also appears on line 42 of the same file.
const proc = spawnProcess(poetry, args, { cwd });
token?.onCancellationRequested(() => {
proc.kill();
reject(new CancellationError());
});
src/managers/poetry/commands/showTopLevel.ts:33
- runPoetry() is awaited outside the try/catch, so failures (missing Poetry, non-zero exit, cancellation) will throw instead of returning the intended empty Set fallback. If this command is meant to be resilient like the existing Poetry package listing code, wrap the runPoetry call in the try/catch as well.
async execute(executeArgs?: PoetryShowTopLevelExecuteArgs): Promise<Set<string>> {
const output = await runPoetry(this.buildCommand(), executeArgs?.cwd, this.log, executeArgs?.cancellationToken);
src/managers/builtin/commands/uninstall.ts:30
- The UV uninstall docstring includes a
-yflag, but UvUninstallCommand.buildCommand() (and the existing uv uninstall path in src/managers/builtin/utils.ts) does not pass-y/--yes. Update the parsed-command documentation to match the actual invocation.
/**
* UV uninstall command.
* Parsed command: `uv pip uninstall -y --python <path> <package>`
* Official documentation: https://docs.astral.sh/uv/pip/
*/
src/managers/base/commands/availableVersions.ts:25
- The prerelease filter regex
!/[ab]|rc|dev/iwill exclude any version containing the letters 'a' or 'b' anywhere (e.g. local versions like1.0+abc), not just PEP 440 prerelease markers. Tighten the check to only match prerelease markers (a, b, rc, dev) that appear immediately after a digit.
protected filterVersions(versions: string[], includePrerelease?: boolean): string[] {
let filtered = versions.filter((v) => !!v.trim());
if (includePrerelease === false) {
filtered = filtered.filter((version) => !/[ab]|rc|dev/i.test(version));
}
src/managers/poetry/commands/show.ts:22
- runPoetry() is awaited before the try/catch, so command failures will throw rather than returning the [] fallback. Move the runPoetry call inside the try so the error path consistently returns an empty list (matching the current catch behavior).
async execute(executeArgs?: PoetryShowExecuteArgs): Promise<PackageInfo[]> {
const output = await runPoetry(this.buildCommand(), executeArgs?.cwd, this.log, executeArgs?.cancellationToken);
const packages: PackageInfo[] = [];
try {
src/managers/builtin/commands/availableVersions.ts:2
- Available-versions commands need pep440 parsing/sorting to satisfy the API contract (newest-first). Add pep440 imports here so PipAvailableVersionsCommand/UvAvailableVersionsCommand can sort versions using rcompare (matching parsePipIndexVersionsJson in src/managers/builtin/pipPackageManager.ts).
import { AvailableVersionsCommand, type AvailableVersionsExecuteArgs } from '../../base/commands/index';
import { runPython, runUV } from '../helpers';
src/managers/builtin/commands/availableVersions.ts:42
- PipAvailableVersionsCommand currently returns versions without enforcing newest-first ordering. The PackageManager API contract requires newest-first (src/api.ts:741-742). Sort the filtered versions using pep440 rcompare (as parsePipIndexVersionsJson does).
This issue also appears on line 83 of the same file.
const parsed = JSON.parse(match[0]) as { versions?: string[] };
return this.filterVersions(
Array.isArray(parsed.versions) ? parsed.versions : [],
executeArgs.includePrerelease,
);
src/managers/conda/commands/availableVersions.ts:2
- CondaAvailableVersionsCommand should return versions newest-first per the API contract (src/api.ts:741-742). Add pep440 imports so the implementation can parse and sort versions consistently with the existing condaPackageManager (src/managers/conda/condaPackageManager.ts:192-194).
import { AvailableVersionsCommand, type AvailableVersionsExecuteArgs } from '../../base/commands/index';
import { runCondaExecutable } from '../condaUtils';
src/managers/conda/commands/availableVersions.ts:27
- This returns a Set of versions but doesn’t sort newest-first and doesn’t apply the includePrerelease filter from AvailableVersionsCommand. Parse and sort via pep440 rcompare to match the API contract and existing conda behavior.
const versions = (parsed[executeArgs.packageName] as Array<{ version?: string }>)
.map((entry) => entry.version?.trim() ?? '')
.filter((version) => !!version);
return Array.from(new Set(versions));
src/managers/poetry/commands/version.ts:26
- This uses this.pythonExecutable as the Poetry executable path. Other Poetry helpers resolve Poetry via getPoetry() (respecting the python.poetryPath setting and discovery). If pythonExecutable is actually a Python interpreter (as the name suggests), this will silently return undefined and mask version detection. Use getPoetry() to locate the Poetry binary, then pass that into getPoetryVersion().
async execute(_executeArgs?: BaseExecuteArgs): Promise<Pep440Version | undefined> {
try {
// Poetry version is obtained via getPoetryVersion utility which handles poetry --version
// We pass the pythonExecutable as the poetry path since it was set to the poetry executable
const versionString = await getPoetryVersion(this.pythonExecutable);
return versionString ? (parsePep440Version(versionString) ?? undefined) : undefined;
b620149 to
91d15c3
Compare
91d15c3 to
689dd84
Compare
689dd84 to
54b13fe
Compare
|
Adding "Skip Tests" label since tests are added in a followup PR for simplicity (this PR only introduces the classes, it does not wire them up still) |
|
Should also fix #1650 |
Summary
Introduces the reusable command-object layer for package management while leaving existing package-manager call sites unchanged.
Scope
PackageManagerCommandbase class and shared execution options.Non-goals
This PR does not add command tests or migrate package managers to the new classes. Those changes are isolated in follow-up PRs #1677 and #1678.
PR stack
main.Testing
npm run lintnpm run compile-testsnpm run unittest(1,448 passing, 4 pending)